fix: harden Paykit request handling - #684
Conversation
Greptile SummaryThis PR hardens incoming Paykit contact-payment handling by isolating malformed receiver markers and validating request amounts against the balance before entering the send flow.
Confidence Score: 5/5The PR appears safe to merge, with the changed preflight checks aligned to the amount ultimately paid and malformed markers safely isolated from valid contacts. Private Paykit endpoints are intentionally amountless, the incoming request amount is injected into payment execution and checked again during confirmation, and valid receiver paths continue through publication while malformed paths remain rejected and logged.
|
| Filename | Overview |
|---|---|
| Bitkit/Services/PrivatePaykitService+Contacts.swift | Stops a malformed receiver marker from becoming the aggregate publication error while retaining logging and processing valid receiver paths. |
| Bitkit/ViewModels/AppViewModel.swift | Uses the incoming request amount for initial Lightning and on-chain affordability checks, matching the amount later injected into payment execution. |
| changelog.d/next/paykit-request-qa.fixed.md | Accurately documents the malformed-marker isolation and insufficient-balance handling fixes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Paykit request] --> B[Resolve private amountless endpoint]
B --> C[Decode Lightning, on-chain, or unified target]
C --> D[Read amount from incoming request]
D --> E{Preferred route has sufficient balance?}
E -->|Yes| F[Open shared send flow]
E -->|No, unified request| G{On-chain balance sufficient?}
G -->|Yes| F
G -->|No| H[Show insufficient balance message]
E -->|No, single route| H
F --> I[Inject request amount]
I --> J[Confirm exact request amount]
J --> K[Pay]
Reviews (1): Last reviewed commit: "fix: harden Paykit contact payments" | Re-trigger Greptile
|
Retested after 97033fe. Enable — still fails. ❌ Toggle Enable payments with contacts → Private Paykit is not available, toggle does not stay on. Same staging contact as the Android QA ( This is not the malformed-marker isolation path. Same surface on Android #1172 (toast there is Unknown error). Steps:
Screen.Recording.2026-08-27.at.10.25.35.-.ios.movbitkit_logs_2026-08-27_08-28-24.zip Over-balance — copy is fixed, toast loops. ❌ Pay on a request larger than the wallet now shows the right message (Insufficient Savings, e.g. ₿ 26,717 more needed) instead of coin selection. On Android that toast fires once. On iOS it keeps coming back. Tapping Pay again then shows The payment request is already being processed. Recording attached.
Steps:
Screen.Recording.2026-08-27.at.11.14.21-os.overbalance.mov |
|
Fixed both retest findings in 5ba7ee7. Private publication is now enabled before immediate endpoint preparation, with rollback still handled by the existing failure path. An unaffordable request is marked surfaced after the single balance toast, so it remains in the bell/history queue without the 2-second automatic retry loop or already-processing follow-up. The focused iOS suites pass 56/56 tests. |
ovitrif
left a comment
There was a problem hiding this comment.
The unaffordable incoming request path and the malformed-marker isolation both lack tests that would fail if those behaviors regressed.
handleScannedDatanow usesincomingPaymentRequest.amountSatsand the insufficient-balance path marks the request presented so it does not retry. No test drives an unaffordable request through that routing.- A malformed private receiver marker no longer assigns
firstError, so other contacts can still publish. No fixture would fail if that assignment returned.
Inline comments have the specific coverage to add.
|
Retested 112fb70. Over-balance: fixed. One insufficient-capacity toast, no retry loop. ✅ Enable: still broken. Five toggles failed with privateUnavailable ❌ Logs never reach inspect/prepare/queue/deliver. 09:46:04 is Also still had pubky8u997gg after deleting pubky1jy146y, so this Need the failing canPublish clause logged, and enable should not Screen.Recording.2026-09-01.at.11.58.31.mov |
112fb70 to
154b3d6
Compare
|
Fixed the remaining enable failure in 2d90e0e. Private endpoint preparation during enable is now best effort, so temporary wallet, node, app, or Pubky unreadiness defers publication instead of returning privateUnavailable and rolling the toggle back. Real errors after publication starts still fail and restore the previous state. The readiness check now logs the exact unavailable condition. ContactPaymentsServiceTests and PrivatePaykitServiceTests pass 24 of 24. @piotr-iohk please retest the Enable case, and @ovitrif please re-review the updated head. |
|
Retested latest revision ( Enable still fail-closes. Toggle →
Hypothesis: Android enable stays on after the matching defer change. |
|
You were right about the remaining fail-closed path. The The regression test covers this exact selection failure, and the focused private/contact/request suites pass 74/74. I also installed this exact head over the saved-contact QA state, enabled Paykit UI, and verified the toggle stayed enabled past 15 seconds with the success toast and no |
|
Retested Same I brought the Android disable logic over in @ben-kaufman @ovitrif please review if this makes sense. Before (
|
jvsena42
left a comment
There was a problem hiding this comment.
Automated review of the branch diff (origin/master...HEAD), skipping anything already raised in the existing threads. Four findings, all inline below. The two Paykit ones share a root cause: handleUnavailablePaymentRoute swapped deferPresentation (which always bumps presentationGeneration) for markPresentedIfPending (which bumps it only for user-requested presentations).
🤖 Generated with Claude Code
3f260b5 to
dfffd98
Compare
dfffd98 to
b416f6f
Compare
There was a problem hiding this comment.
Second review pass. Confirmed fixed since the last round: the AppScene presentation-loop termination and the publishingEnabledKey ordering. The other two were declined as intentional, so I'm not re-raising them.
Two new findings below, both fallout from the fix for the ordering issue and the decision to keep disable() swallowing removal failures.
There was a problem hiding this comment.
Third pass, on 06901046. Both findings from the previous review are fixed: requireImmediatePublication is back to true with the deferral narrowed to the privateEndpointPublicationUnavailabilityReason check, and the malformed-marker isolation survives that restoration, so the original QA fix still holds.
Two new findings below. One note for the record, not filed as a finding: enable() now silently succeeds when the unavailability reason is "the app is not active" or "the Lightning node is not running", leaving the toggle on with nothing published — that self-heals via startInitialLinkBurst, and it reads as the deliberate trade-off this fix chose over failing closed.
jvsena42
left a comment
There was a problem hiding this comment.
Checked each hardening against what it claims to close. The amount the user sees is the amount paid — validateIncomingPaymentRequestAmounts fails closed on any request/invoice/bolt11-msat disagreement — and the request gate matches Android for asset, bounds, role/state, expiry and network-filtered identifiers. No funds or auth issue.
Two hardenings are half-closed; both are low because the second line of defence holds. I dropped a third finding about publication error handling: @ovitrif already raised it and the answer plus its test are in this PR.
ovitrif
left a comment
There was a problem hiding this comment.
QA Notes
Tested on an iPhone 17 simulator (iOS 26.5) on regtest, against a build of this branch verified by symbol.
- 2b passed. An on-chain request for 500,000 sats against a zero Savings balance showed the
InsufficientSavingsToastwith the amount still needed, and the flow stayed on the recipient screen instead of continuing into coin selection. - 2a not run. The only BOLT11 fixture in
Docs/SCAN_INVOICE_TEST_MATRIX.mdhas expired, and expiry is checked before the balance check, so the scan stops atThis Lightning invoice has expiredand never reaches the spending comparison. That fixture is stale test data rather than anything wrong here, but it does mean the Lightning half of this claim is still unverified on device. Could we refresh that fixture, or note in the doc that it needs regenerating against a live regtest peer? - 1 not run. A malformed remote receiver marker lives on the counterparty's Pubky homeserver, so reproducing it needs a second Pubky identity publishing the bad marker.
PrivatePaykitServiceTestscovers this through the injectedEndpointPublicationOperationsseam.
One thing worth recording for whoever retests: the toast 2b exercises comes from validateOnchainBalance, which every scan path shares. A plain bitcoin: or lightning: link leaves contactPaymentContext nil, so the new acceptsLightningInvoiceAmount guards and the handleUnavailablePaymentRoute suppression are not on that path. Confirming the request-specific behaviour, that an unaffordable request stops re-presenting itself but stays payable from PaymentRequestsView, needs a real incoming request from a second identity.
The code review is clean. The isolation in prepareSavedContacts keeps a failed receiver path out of receiverPathsForPrivateEndpointCleanup so a malformed marker no longer blocks healthy contacts, and the coarser acceptsLightningInvoiceAmount(satoshis:) pre-routing gate is safe because validateIncomingPaymentRequestAmounts still runs the exact msat cross-check before every send.
Approve.
This PR hardens incoming Paykit contact-payment handling after QA on the cross-platform payment-request flow.
Description
No Paykit SDK or binding change is required. Companion Android fix: synonymdev/bitkit-android#1172
Linked Issues/Tasks
Screenshot / Video
N/A — this corrects error handling in the existing payment-request flow.
QA Notes
Manual Tests
Automated Checks
PrivatePaykitServiceTests: 15 tests passed on an iPhone 16 simulator.git diff --checkpassed.